Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(lightning): Upgrade To Blocktank v2 API #1216

Merged
merged 37 commits into from
Sep 4, 2023
Merged

Conversation

coreyphillips
Copy link
Collaborator

Description

Upgrades Bitkit to the Blocktank v2 API here.

@coreyphillips coreyphillips marked this pull request as draft August 22, 2023 15:45
@coreyphillips
Copy link
Collaborator Author

@limpbrains, any thoughts on why this test might be failing?

@limpbrains
Copy link
Collaborator

test expected QuickSetupReserveNote to be shown, but in reality QuickSetupBlocktankNote is shown. I've changed test to reflect it.
I've removed all sleep(1000), it is really not needed, when you run test with E2E=true.
Also test was failing with [BlocktankClientError: Failed to create order. "Channel size too big. Max size is USD999."] on the last step when buying big barrel on CustomSetup screen. Looks like a rounding bug in Bitkit ot Blocktank. I changed test to use medium barrel - $500. Maybe this needs to be fixed and reverted?

Here is a video of the test run:

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-08-25.at.10.32.53.mp4

@coreyphillips
Copy link
Collaborator Author

coreyphillips commented Aug 25, 2023

Looks like the total channel size (incoming + receiving) was exceeding the maxChannelSizeSat value when the user opted for a spending value + the max receiving value in the custom flow. So in this commit and this commit we subtract any spending amount from the max receiving to ensure it remains under that limit in both Quick & Custom setups.

@coreyphillips
Copy link
Collaborator Author

I no longer understand how to appease the testing gods as @limpbrains did, but it's at least ready for review. @pwltr

@coreyphillips coreyphillips marked this pull request as ready for review August 25, 2023 18:48
@limpbrains
Copy link
Collaborator

Slider on the quick setup screen feels broken

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-08-26.at.09.21.50.mp4

@limpbrains
Copy link
Collaborator

Testing Gods are Pleased

@pwltr
Copy link
Collaborator

pwltr commented Aug 31, 2023

The custom setup flow is broken when selecting the maximum local balance:

Simulator.Screen.Recording.-.iPhone.14.-.2023-08-31.at.18.17.02.mp4

Updates receiving package calculations.
Updated blocktankPurchaseFee calculation.
@coreyphillips
Copy link
Collaborator Author

Updated the calculations in this commit. Custom channels should get calculated correctly now no matter how many sats the user has.

Updates receiving package calculations.
@pwltr
Copy link
Collaborator

pwltr commented Sep 1, 2023

Fee is still incorrect here:

Untitled

@pwltr
Copy link
Collaborator

pwltr commented Sep 1, 2023

I opened a channel with 80% spending balance and my receiving capacity is very small. I believe BT should always give me a balanced channel.

Simulator Screenshot - iPhone 14 - 2023-09-01 at 12 50 10

Updates channel fee value in CustomSetup.tsx.
Updates useEffect in QuickSetup.tsx to set max value on load.
@coreyphillips
Copy link
Collaborator Author

I'm surprised Blocktank allowed that channel purchase. The new api usually does not allow the client balance to exceed the lsp balance with the following error: "clientBalanceSat is too big compared to lspBalanceSat. Max ratio: 1.".

In the latest commit, I've updated the QuickSetup component to set the max value on load to prevent the issue you posted above. I also adjusted the Cost: value to reflect the fee instead of the total sats needed to send to Blocktank.

Should be good to go for testing, just need to update the test script so that it passes with the new values for lightning onboarding.

@pwltr
Copy link
Collaborator

pwltr commented Sep 1, 2023

In the latest commit, I've updated the QuickSetup component to set the max value on load to prevent the issue you posted above.

I don't think we want to default to 80% from a design perspecive. Also it doesn't resolve the issue with the unbalanced channel for me.

I also adjusted the Cost: value to reflect the fee instead of the total sats needed to send to Blocktank.

This still doesn't show the correct amount:

Simulator Screenshot - iPhone 14 - 2023-09-01 at 15 31 26
Simulator Screenshot - iPhone 14 - 2023-09-01 at 15 31 29

@coreyphillips
Copy link
Collaborator Author

We've made it pretty far in this PR. I think we should call a checkpoint here so we can get some additional testing in with the new api and apply needed fixes in separate PR's from there

The Cost is the Blocktank fee. I don't believe we're able to calculate the cost of the transaction at this point. We might want to change the verbiage to "Blocktank Fee" or something similar.

The 80% is part of a larger discussion especially as anchor channels emerge.

@pwltr
Copy link
Collaborator

pwltr commented Sep 1, 2023

The Cost is the Blocktank fee. I don't believe we're able to calculate the cost of the transaction at this point. We might want to change the verbiage to "Blocktank Fee" or something similar.

This is working on the master branch. Unless I'm missing something I don't see the reason to introduce a regression here.

The 80% is part of a larger discussion especially as anchor channels emerge.

I don't understand the need for this change. The unbalanced channel issue is independent of the 80% limit. I'm unable to get a balanced channel with any spending amount. This really needs to be fixed otherwise BT v2 channels will be pretty much useless for receiving.

@coreyphillips
Copy link
Collaborator Author

The fee Cost discrepancy should be sorted as of the latest set of commits.

@coreyphillips
Copy link
Collaborator Author

Also increased the lsp balance when setting 0 sats on the clients end to something more usable. Should be good to go.

@pwltr
Copy link
Collaborator

pwltr commented Sep 4, 2023

Channel balancing seems better now, it's still defaulting to 80% spending balance in QuickSetup though. From my tests that doesn't affect balancing at all, and it's not in the design this way, so this should be reverted. Also there are a lot of leftover console logs.

@coreyphillips
Copy link
Collaborator Author

@pwltr Could you expand on this? Do you mean it's defaulting to a value greater than $1000 at 80%?
"It's still defaulting to 80% spending balance in QuickSetup though. From my tests that doesn't affect balancing at all, and it's not in the design this way"

@pwltr
Copy link
Collaborator

pwltr commented Sep 4, 2023

I'm talking about the slider on the QuickSetup screen, you've changed it to be set at 80% initially for a reason that is not apparent to me. It should be set to 20%.

image

Sets default client balance to 20%, if able, instead of 80%.
@coreyphillips
Copy link
Collaborator Author

Gotcha. Updated here. Changed it from setting it to the max of 80% to 20%.

Copy link
Collaborator

@pwltr pwltr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of nitpicks other than that LGTM

src/store/actions/blocktank.ts Outdated Show resolved Hide resolved
src/store/actions/blocktank.ts Outdated Show resolved Hide resolved
src/store/actions/blocktank.ts Outdated Show resolved Hide resolved
src/store/actions/blocktank.ts Outdated Show resolved Hide resolved
@coreyphillips coreyphillips merged commit d914ce7 into master Sep 4, 2023
3 of 4 checks passed
@coreyphillips coreyphillips deleted the blocktank-v2 branch September 4, 2023 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants